Skip to content

refactor!: remove export_metrics feature and related configuration/docs#6925

Closed
kumarUjjawal wants to merge 5 commits into
GreptimeTeam:mainfrom
kumarUjjawal:eleminate-export
Closed

refactor!: remove export_metrics feature and related configuration/docs#6925
kumarUjjawal wants to merge 5 commits into
GreptimeTeam:mainfrom
kumarUjjawal:eleminate-export

Conversation

@kumarUjjawal
Copy link
Copy Markdown

@kumarUjjawal kumarUjjawal commented Sep 7, 2025

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

This PR is related to the issue #6907

What's changed and what's your intention?

As discussed in the issue #6907 the proposal was to Eliminate the export_metrics functionality and related documentation (including the standalone page) and remove the information_schema.runtime_metrics table. I have hunt down all the refrences and removed as per my knowledge.

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

@kumarUjjawal kumarUjjawal requested review from a team and MichaelScofield as code owners September 7, 2025 11:24
@github-actions github-actions Bot added size/M docs-not-required This change does not impact docs. labels Sep 7, 2025
@WenyXu
Copy link
Copy Markdown
Member

WenyXu commented Sep 8, 2025

Hi @kumarUjjawal There are some conflicts

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 8, 2025

Hi @kumarUjjawal There are some conflicts

I will resolve those soon.

@github-actions github-actions Bot added size/S and removed size/M labels Sep 8, 2025
@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 8, 2025

@WenyXu can you approve the workflow?

@killme2008 killme2008 changed the title refactor: remove export_metrics feature and related configuration/docs refactor!: remove export_metrics feature and related configuration/docs Sep 9, 2025
@WenyXu
Copy link
Copy Markdown
Member

WenyXu commented Sep 9, 2025

@WenyXu can you approve the workflow?

Hi @kumarUjjawal, some CI checks have failed. Please follow the CI output for details. For Clippy and rustfmt issues, you can run make clippy and make fmt locally to reproduce them.

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 9, 2025

@WenyXu can you approve the workflow?

Hi @kumarUjjawal, some CI checks have failed. Please follow the CI output for details. For Clippy and rustfmt issues, you can run make clippy and make fmt locally to reproduce them.

Definitely, I will fix those.

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 9, 2025

@WenyXu There are mentions of export_metrics in the config also the server crate contains the export_metrics.rs and the self contained tests, should I remove those as well.

@WenyXu
Copy link
Copy Markdown
Member

WenyXu commented Sep 9, 2025

@WenyXu There are mentions of export_metrics in the config also the server crate contains the export_metrics.rs and the self contained tests, should I remove those as well.

Yes

@github-actions github-actions Bot added size/M and removed size/S labels Sep 9, 2025
@evenyag
Copy link
Copy Markdown
Contributor

evenyag commented Sep 9, 2025

Hi, please fix the CI checks and sign your commits.

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 9, 2025

Hi, I found three variants of SNAFU error that are defined but never used anywhere, that's why one of the checks was failing, what is your preferences for that, should I remove those?

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 9, 2025

The thee failing tests doesn't seem to come from my changes, Can you check?
./tests/cases/distributed/common/view/create
./tests/cases/distributed/common/system/information_schema
./tests/cases/distributed/common/show/show_databases_tables

@evenyag
Copy link
Copy Markdown
Contributor

evenyag commented Sep 10, 2025

Hi, I found three variants of SNAFU error that are defined but never used anywhere, that's why one of the checks was failing, what is your preferences for that, should I remove those?

Sure, it's fine to remove them.

The thee failing tests doesn't seem to come from my changes, Can you check?
./tests/cases/distributed/common/view/create
./tests/cases/distributed/common/system/information_schema
./tests/cases/distributed/common/show/show_databases_tables

They are related to your changes. You should update the sqlness result because you have removed a table from the database.
https://docs.greptime.com/contributor-guide/tests/sqlness-test/

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 10, 2025

Hi, I found three variants of SNAFU error that are defined but never used anywhere, that's why one of the checks was failing, what is your preferences for that, should I remove those?

Sure, it's fine to remove them.

The thee failing tests doesn't seem to come from my changes, Can you check?
./tests/cases/distributed/common/view/create
./tests/cases/distributed/common/system/information_schema
./tests/cases/distributed/common/show/show_databases_tables

They are related to your changes. You should update the sqlness result because you have removed a table from the database. https://docs.greptime.com/contributor-guide/tests/sqlness-test/

Oh! Thanks for the docs.

@kumarUjjawal
refactor: remove export_metrics feature and related configuration/docs
92d20bb 
Signed-off-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
@kumarUjjawal
fixed clippy issues
a858890 
Signed-off-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
@kumarUjjawal
removed remaining mentions of export metrics and fixed clippy issues
177dc74 
Signed-off-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
@kumarUjjawal
removed unused SNAFU error and ran sqlness test
7a8531c 
Signed-off-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
@evenyag
Copy link
Copy Markdown
Contributor

evenyag commented Sep 16, 2025

Hi, the unit test failed.

You also have to update this test:

greptimedb/tests-integration/tests/http.rs

Lines 1480 to 1482 in db42ad4

[export_metrics]
enable = false
write_interval = "30s"

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Sep 16, 2025

Hi, the unit test failed.

You also have to update this test:

greptimedb/tests-integration/tests/http.rs

Lines 1480 to 1482 in db42ad4

[export_metrics]
enable = false
write_interval = "30s"

Will removing these line fix the issue or do I need to make other changes as well?

@evenyag
Copy link
Copy Markdown
Contributor

evenyag commented Sep 17, 2025

Hi, the unit test failed.
You also have to update this test:

greptimedb/tests-integration/tests/http.rs

Lines 1480 to 1482 in db42ad4

[export_metrics]
enable = false
write_interval = "30s"

Will removing these line fix the issue or do I need to make other changes as well?

You can run make check to ensure all tests are passed, or check the CI status and fix failures.

@kumarUjjawal
Copy link
Copy Markdown
Author

kumarUjjawal commented Oct 6, 2025

Update: I have been busy with work that's why I was unable to make any progress I will probably resolve this by the next weekend.

@MichaelScofield MichaelScofield marked this pull request as draft October 21, 2025 03:27
@MichaelScofield
Copy link
Copy Markdown
Collaborator

MichaelScofield commented Nov 18, 2025

duplicated: #7236

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelScofield MichaelScofield Awaiting requested review from MichaelScofield MichaelScofield is a code owner

@WenyXu WenyXu Awaiting requested review from WenyXu WenyXu will be requested when the pull request is marked ready for review WenyXu is a code owner

Assignees

No one assigned

Labels

docs-not-required This change does not impact docs. size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kumarUjjawal @WenyXu @evenyag @MichaelScofield